Skip to content

fix(statedb): preserve detected session-id keys on stale full-table saves#1526

Merged
asheshgoplani merged 1 commit into
asheshgoplani:mainfrom
lazyoft:feature/t-0133-investigate-mapping
Jul 12, 2026
Merged

fix(statedb): preserve detected session-id keys on stale full-table saves#1526
asheshgoplani merged 1 commit into
asheshgoplani:mainfrom
lazyoft:feature/t-0133-investigate-mapping

Conversation

@lazyoft

@lazyoft lazyoft commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Problem

A freshly-launched session's claude_session_id (and the gemini/opencode/codex equivalents) can be silently wiped from state.db, surfacing as "no session ID detected" on a session whose Claude transcript is actively being written. Callers that resolve a session's transcript by its persisted id (e.g. a coordinator mapping its workers to ~/.claude/projects/<uuid>.jsonl) intermittently lose the mapping within seconds of launch.

Root cause

The per-tool conversation ids are detected asynchronously and persisted shortly after launch. But any concurrent full-table saveSaveWithGroupsSaveInstancessaveInstancesOnce — rebuilds each row's tool_data from its in-memory Instance. If that writer's snapshot has not yet observed the id, the rebuilt blob omits it (omitempty zero-value).

MergeToolDataExtras preserved only keys unknown to the typed schema; for a known key it let the new blob win "including absence-by-omitempty". So an omitted id cleared the previously-persisted value instead of being preserved.

The hottest such writer is the rename hook (hook_name_sync.go): on every Claude hook that renames a session it does LoadWithGroups then SaveWithGroups over the whole snapshot. Other full-rewrite writers (internal/ui/home.go, web_mutator.go, group_cmd.go, session_move.go) hit the same path less frequently. The launch path itself is already hardened (InsertSessionAndVerify, #1031), so the clobberer is always a different writer.

Fix

Treat the detected session-id keys as sticky in MergeToolDataExtras: {claude,gemini,opencode,codex}_session_id and their *_detected_at. When the new blob omits a sticky key but the old row has it, carry the old value forward. Semantics:

  • non-empty new value still wins — a real resume/fork that changes the id;
  • explicit empty present in the new blob ("claude_session_id":"") is honored as an intentional clear;
  • only outright omission preserves.

This is a one-line change to the merge guard (if known[k] && !sticky[k]) plus a small key set; it does not touch the save/sweep contract.

Design note

Deliberate clears must now use an explicit empty (or a single-row clear) rather than relying on an omit. UpdateClaudeSessionsWithDedup clears in-memory only; with sticky keys a cleared id is restored from disk on reload and re-cleared in memory each load. Since duplicates "shouldn't occur in normal use" and tmux env is authoritative, that is a harmless redundant in-memory re-dedup, not a functional regression.

Tests

internal/statedb/tool_data_extras_test.go:

  • repoints the typed-absence test to a non-sticky key (latest_prompt), since claude_session_id absence behavior intentionally changed;
  • adds sticky coverage: preserve-on-omit (all four tools + a *_detected_at), non-empty-new-wins, and explicit-empty-clears.

go test ./internal/statedb/ passes; gofmt/go vet clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved how session-related data is merged so previously saved values are retained when newer data omits them.
    • Non-sticky fields still follow the usual overwrite rules, and explicit empty values are respected as intentional clears.
  • Tests
    • Expanded test coverage for missing, updated, and intentionally cleared session-related values during merges.

…aves

A freshly-launched session's claude_session_id (and the gemini/opencode/codex
equivalents) could be silently wiped from state.db, surfacing as
"no session ID detected" on a session whose transcript is actively being
written.

Root cause: the per-tool conversation ids are detected asynchronously and
persisted shortly after launch, but any concurrent full-table save
(SaveWithGroups -> SaveInstances) whose in-memory snapshot has not yet
observed the id rebuilds tool_data with the id omitted (omitempty
zero-value). MergeToolDataExtras treated every typed/known key as
authoritative "including absence", so the omitted id cleared the
previously-persisted value instead of being preserved. The rename hook
(hook_name_sync) is the hottest such writer; it fires on every Claude hook.

Fix: treat the session-id keys ({claude,gemini,opencode,codex}_session_id
and their *_detected_at) as sticky in MergeToolDataExtras. When the new
blob OMITS a sticky key but the old row has it, carry the old value
forward. A non-empty new value still wins (a real resume/fork that changes
the id), and an explicit empty present in the new blob is honored as an
intentional clear -- only outright omission preserves.

Extends tool_data_extras_test.go: repoints the typed-absence test to a
non-sticky key (latest_prompt) and adds sticky coverage for preserve-on-
omit, non-empty-new-wins, and explicit-empty-clears.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wm3xPWp2Wm5B5QjSHkEnXD
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bd8e1e0a-d4b9-4d99-ad3e-82b8dc65f14f

📥 Commits

Reviewing files that changed from the base of the PR and between 0e5ae31 and cd781b5.

📒 Files selected for processing (2)
  • internal/statedb/tool_data_extras.go
  • internal/statedb/tool_data_extras_test.go

📝 Walkthrough

Walkthrough

MergeToolDataExtras now preserves selected session-id and detected-at JSON fields when new tool data omits them, while still allowing explicit new values and explicit empty values to replace existing ones. Tests were updated for omission, override, and explicit-clear cases.

Changes

Sticky session-id merge semantics

Layer / File(s) Summary
Sticky merge contract
internal/statedb/tool_data_extras.go
Documentation, the sticky-key list, and the merge loop now treat the listed session-id and timestamp keys as preserved-on-omission exceptions.
Merge behavior tests
internal/statedb/tool_data_extras_test.go
Tests now cover non-sticky omission, sticky omission preservation, non-empty override, and explicit empty clear behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • asheshgoplani/agent-deck#1140: Both changes touch tool_data session-id fields and their companion timestamps during merge and persistence handling.

Suggested reviewers

  • asheshgoplani
🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 70.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test_coverage_per_surface ⚠️ Warning Only internal/statedb unit tests cover the sticky merge; no CLI/TUI/Web/remote regression tests assert the stale-save omit-preserves contract at any surface. Add at least one regression per applicable surface (or a shared harness exercised by CLI/TUI/Web/remote) that forces a stale save and verifies sticky session IDs survive.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses Conventional Commits format and accurately describes the merge behavior fix for sticky session-id keys.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Remote_parity ✅ Passed The PR only changes internal/statedb merge logic and tests; no internal/ui/ or cmd/agent-deck TUI code is part of the patch, so the RemoteSession parity check is not applicable.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Owner

🤖 autopilot: CI green + Claude review clean; FLAGGED for local Codex dual-review before merge (data-sensitive area).

The change modifies MergeToolDataExtras in internal/statedb/tool_data_extras.go to preserve sticky session-id keys (claude_session_id, gemini_session_id, etc.) when the new blob omits them — fixing the t-0133 race where a concurrent full-table save could wipe a live session's mapping. The logic is correct and well-tested, but it touches the statedb merge path which is directly on the data-critical layer. Holding for local Codex dual-review per policy.


Generated by Claude Code

Copy link
Copy Markdown
Owner

🤖 autopilot: CI green + Claude review clean; FLAGGED for local Codex dual-review before merge (data-sensitive area).

Review summary: The fix correctly reorders the sticky-key preservation logic in MergeToolDataExtras so that detected session-id keys (claude_session_id, etc.) are not wiped by concurrent full-table saves. Logic is sound, test coverage is thorough, and no data-loss risk is introduced. However, this touches internal/statedb directly on the merge path for every save, which puts it in the data-sensitive category requiring a second local review pass before autopilot merge.


Generated by Claude Code

Copy link
Copy Markdown
Owner

🤖 autopilot: CI green + Claude review clean; FLAGGED for local Codex dual-review before merge (data-sensitive area: internal/statedb sticky-key merge logic).


Generated by Claude Code

Copy link
Copy Markdown
Owner

🤖 autopilot 2026-06-29: CI still green; still FLAGGED for local Codex dual-review before merge (data-sensitive area — internal/statedb/tool_data_extras.go). No change from yesterday's flag.


Generated by Claude Code

Copy link
Copy Markdown
Owner

🤖 autopilot: CI green + Claude review clean; FLAGGED for local Codex dual-review before merge (data-sensitive area: statedb tool_data merge logic for session-ID sticky keys — modifies the MergeToolDataExtras path). Codex should verify that omitempty JSON tags produce full key-absence (not "") for zero values, to ensure the three-way omit/new-wins/explicit-clear logic works as intended.


Generated by Claude Code

Copy link
Copy Markdown
Owner

🤖 autopilot: CI green + Claude review clean; FLAGGED for local Codex dual-review before merge (data-sensitive area). This PR modifies internal/statedb/statedb.go (new SetArchived targeted UPDATE) and internal/statedb/tool_data_extras.go (sticky session-id keys) — statedb changes require local dual-review per autopilot policy.


Generated by Claude Code

@asheshgoplani asheshgoplani merged commit a9ea185 into asheshgoplani:main Jul 12, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants